fix: Allow nullable Exclude properties#198
fix: Allow nullable Exclude properties#198Dreamescaper wants to merge 1 commit intoartiomchi:mainfrom
Conversation
| { | ||
| var matchProperties = _matchExpression != null | ||
| ? ProcessPropertiesExpression(_entityType, _matchExpression, true) | ||
| ? ProcessPropertiesExpression(_entityType, _matchExpression!, true) |
There was a problem hiding this comment.
Wait, is this line required here?
There was a problem hiding this comment.
Yeah, it complains about conversion without !:
Argument of type 'Expression<Func<TEntity, object>>' cannot be used for parameter 'propertiesExpression' of type 'Expression<Func<TEntity, object?>>' in 'IProperty[] UpsertCommandBuilder.ProcessPropertiesExpression(IEntityType entityType, Expression<Func<TEntity, object?>> propertiesExpression, bool match)' due to differences in the nullability of reference types.
I thought about making _matchExpression object? as well, but probably it shouldn't ever point to nullable property.
There was a problem hiding this comment.
It might be worth validating that through tests, as the expression might be inflated on in memory EF
There was a problem hiding this comment.
Could you clarify what to validate exactly?
I thought about adding integration test with nullable Exclude property, but currently Nullable=false for integration tests project.
There was a problem hiding this comment.
Hmm.. good point - in which case it's probably safe to just merge it :D
I'll probably run a test (or check if we have one that won't have issues excluding columns that have null values). It shouldn't, but I want to try sanity check just to cover all bases
There was a problem hiding this comment.
I thought about making _matchExpression object? as well, but probably it shouldn't ever point to nullable property.
Ah, I assume you mean this sentence. It's not that it wouldn't work or fail if matchExpression leads to nullable property - ProcessPropertiesExpression doesn't care if it's nullable or not, it simply inspects expression, that's it.
It's just semantically matchExpression leads to PK or unique index, so the column shouldn't be nullable.
Currently the following code produces nullable warning:
It works perfectly fine if you use
.Exclude(m => m.Description!).All I did was to change
Expression<Func<TEntity, object>>toExpression<Func<TEntity, object?>>for Exclude expressions.